Skip to content

Conversation

pumpkinball
Copy link
Contributor

Fixes Issue 272 when returning Employee DateofBirthAsDate, now handles birth years before 1970

@CodeSmith32
Copy link

@pumpkinball
Thanks! This is a much needed fix! But there are a few issues with this PR:

  1. The comma in the character group, line 39, is wrong. Timezones only start with + and -, not ,. Should just be [+-].

  2. This PR may create a new issue: When the date does not contain a timezone offset, the code throws an exception. According to Xero's documentation, dates may take the form, '/Date(1439434356790)/' or '/Date(1419937200000+0000)/', but this PR does not handle the former case. If $matches[2] is empty, presumably you need fall back to '+0000' before creating the DateTimeZone, e.g., new \DateTimeZone($matches[2] ?: '+0000').

  3. Please fix the broken return type as well, line 30. It says @return $this which was never correct even before this PR, and screws up intellisense. Should be @return \DateTime.

  4. Not critical, but perhaps consider what happens if the pattern doesn't match. The previous version raises a warning but returns a zero date, 1970-01-01 00:00:00.000000. This version raises 3 warnings, and (once the timezone issue is fixed) will also return a zero date. Maybe the safest approach would be to raise a single, clearer warning, and return a zero date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants